Add AuxHtlcValidator
#10434
Add AuxHtlcValidator
#10434
Conversation
Summary of ChangesHello @GeorgeTsagk, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a robust mechanism to enhance the reliability of HTLC additions within Lightning channels. It addresses a potential race condition where payment bandwidth calculations could become outdated by implementing a new Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an AuxHtlcValidator to perform a final bandwidth check right before an HTLC is added to the channel state. This is a solid approach to prevent race conditions arising from stale bandwidth information during pathfinding. The implementation is clean and integrates well with the existing channel logic. My main feedback is the lack of unit tests for this new validation logic, which would be beneficial to ensure its correctness and cover edge cases. I've also included one minor suggestion for code simplification.
peer/brontide.go
Outdated
| peerBytes := p.IdentityKey().SerializeCompressed() | ||
| peer, err := route.NewVertexFromBytes(peerBytes) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create vertex from peer "+ | ||
| "pub key: %w", err) | ||
| } |
There was a problem hiding this comment.
This block can be simplified by using route.NewVertex. The current implementation serializes the public key to bytes, then route.NewVertexFromBytes parses it back to a public key for validation before converting it to a route.Vertex. Since p.IdentityKey() is guaranteed to return a valid *btcec.PublicKey, we can use route.NewVertex directly. This is slightly more efficient and makes the code cleaner by removing the unnecessary error handling.
| peerBytes := p.IdentityKey().SerializeCompressed() | |
| peer, err := route.NewVertexFromBytes(peerBytes) | |
| if err != nil { | |
| return fmt.Errorf("failed to create vertex from peer "+ | |
| "pub key: %w", err) | |
| } | |
| peer := route.NewVertex(p.IdentityKey()) |
Roasbeef
left a comment
Roasbeef
left a comment
There was a problem hiding this comment.
My gut reaction: can we get rid of some of the extra calls that validate this elsewhere (eg: switch method calls into the link to check if an HTLC is ready for transit) if we're adding this additional layer of protection?
We definitely need more than 1 call sites per operation (forward / payment) as we need to first quickly gauge if enough funds exist to go ahead with the operation, then verify things one last time before committing it to the channel. The aux bandwidth calls though seem to be a bit intertwined. By adding the Will update PR soon |
GeorgeTsagk
force-pushed
the
extra-aux-band-check
branch
from
283874d to
aea7673
Compare
GeorgeTsagk
force-pushed
the
extra-aux-band-check
branch
from
aea7673 to
a3ebf57
Compare
GeorgeTsagk
force-pushed
the
extra-aux-band-check
branch
from
a3ebf57 to
b29a104
Compare
ziggie1984
left a comment
ziggie1984
left a comment
There was a problem hiding this comment.
Interesting approach, this solves all your flakes on the tap side ?
Previously we'd perform aux bandwidth checks during path finding. This could lead to issues where multiple HTLCs where querying the same bandwidth but were not accounting for each other before being added to the commitment log. We now add a new validator function that will serve as the last point of checks before adding the HTLC to the commitment. During path finding HTLCs could query channel bandwidth asynchronously. At this new call site all HTLCs that are about to be added to the channel have been organised in sequence, so it's safe to query bandwdith again at this point as we're getting the actual up-to-date values. We remove the aux bandwidth check from the helper canSendHtlc, which was called from CheckHTLCTransit and CheckHTLCForward (both are methods of the htlcswitch). For forwards we now fail at the link level, following the introduction of the AuxHtlcValidator. For payments, we now may fail either at the pathfinding level, or at the link level. The htlcswitch may no longer fail for aux bandwidth checks. Finally, when fetching the latest htlc view (for bandwidth checks during pathfinding) we'd silently set the nextHeight of the view to the default zero value. We now make sure to set it to the correct nextHeight value.
When instantiating the lightning channel we now pass in the created HTLC validator. This validator simply performs a bandwidth check and errors out if that is insufficient.
We remove the aux bandwidth check from the helper canSendHtlc, which was called from CheckHTLCTransit and CheckHTLCForward (both are methods of the htlcswitch). For forwards we now fail at the link level, following the introduction of the AuxHtlcValidator. For payments, we now may fail either at the pathfinding level, or at the link level. The htlcswitch may no longer fail for aux bandwidth checks.
We add this constructor for an AuxHtlcDescriptor that allows setting some of the internal fields. This is useful for testing purposes for code external to this package that may need to extensively test the AuxHtlcView.
|
@Roasbeef: review reminder |
GeorgeTsagk
force-pushed
the
extra-aux-band-check
branch
from
b29a104 to
55d1579
Compare
4 participants
Description
Updates the lightning channel to query the TrafficShaper bandwidth once more before adding the HTLC to the channel state. During pathfinding, the reported payment bandwidth could be stale, as it may have not accounted for HTLCs that have not yet been added to the channel state (i.e the aux htlc view).
By querying the aux bandwidth once more, right before the HTLC is added to the channel state, we ensure that no race condition can lead to unexpected failures due to insufficient balance.